Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Relayout after recalculating to ensure visibility changes are handled correctly #7242

Merged
merged 3 commits into from
Dec 13, 2016

Conversation

ivovandongen
Copy link
Contributor

@ivovandongen ivovandongen commented Nov 30, 2016

Fixes #7241
Fixes #7225
Fixes #6758

Re-ordering relayout() after recalculate() ensures that sources are enabled before checking if tiles need to be reloaded on them.

@ivovandongen ivovandongen added the Core The cross-platform C++ core, aka mbgl label Nov 30, 2016
@ivovandongen ivovandongen self-assigned this Nov 30, 2016
@mention-bot
Copy link

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

@ivovandongen ivovandongen added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 30, 2016
@ivovandongen ivovandongen mentioned this pull request Nov 30, 2016
@ivovandongen
Copy link
Contributor Author

@kkaefer @brunoabinader this pr fixes the visibility issue without revering the optimisation in 9127299

A review would be welcome so we can cherry pick it into the android 4.2.0 release

Copy link
Member

@brunoabinader brunoabinader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivovandongen test case looks sounds. This actually fixed the same issue I had with the runtime style example in our Qt demos. Awesome!

@1ec5
Copy link
Contributor

1ec5 commented Nov 30, 2016

This also needs to be cherry-picked to the iOS release branch.

/cc @boundsj

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the test case in mapbox-gl-test-suite instead, so both gl-js and gl-native benefit?

@ivovandongen ivovandongen changed the title Relayout after recalculating to ensure visibility changes are handled correcty Relayout after recalculating to ensure visibility changes are handled correctly Dec 2, 2016
@ivovandongen
Copy link
Contributor Author

@jfirebaugh I fixed the unit test. Not sure how to implement the same thing in a regression test: mapbox/mapbox-gl-test-suite#188

jfirebaugh and others added 3 commits December 12, 2016 13:59
This ensures that a "wait" command will always fully flush pending update flags. This was not the case with the prior conditional map.loaded() logic.
Previously, for viewport sizes less than 512 pixels in either direction, the computed size was 0.
Style::relayout uses source.baseImpl->loaded, a flag which is updated by Style::recalculate. So recalculate first, then relayout.
@jfirebaugh jfirebaugh merged commit 28be37f into master Dec 13, 2016
@jfirebaugh jfirebaugh deleted the 7241-visibility-changes branch December 13, 2016 00:49
boundsj added a commit that referenced this pull request Dec 14, 2016
This ports #7242
commits/2d323211af54499d5c822b8e45d7415bf92112f0 to the iOS 3.4.0
release branch.
boundsj added a commit that referenced this pull request Dec 14, 2016
This ports #7242
commits/2d323211af54499d5c822b8e45d7415bf92112f0 to the iOS 3.4.0
release branch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants